-
Notifications
You must be signed in to change notification settings - Fork 25.7k
Add profile details #123388
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add profile details #123388
Conversation
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
| StringBuilder sb = new StringBuilder(); | ||
| sb.append(this.getClass().getSimpleName()).append("["); | ||
| sb.append("maxPageSize = ").append(maxPageSize); | ||
| sb.append("shards = ").append(sortedUnion(processedShards, sliceQueue.remainingShards())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it is also worth including it in the status object as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about having it in the status only, instead of the toString? I imagine toString is a bit like an identity. So the decision would be whether we view this is part of the identify?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it should be. Each LuceneOperator is only used with list of shards it is originally initialized with
craigtaverner
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Had some minor comments, but nothing important.
|
|
||
| public Iterable<LuceneSlice> getSlices() { | ||
| return slices; | ||
| public Collection<String> remainingShards() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we name this method remainingShardIds, since it returns strings, not shards?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that might be confused with org.elasticsearch.index.shard.ShardId. Probably this should be remainingShardIdentifiers() as it relies on shardContext().shardIdentifier()
This adds list of target shards to operator description.